-
Notifications
You must be signed in to change notification settings - Fork 283
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test(connector-fabric): combine 3 tests into connector-fabric-baseline #3520
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with small nitpick.
BTW When I was creating iroha2 tests I was thinking about using common ledger setup for all the tests, I even moved env setup to separate class for that reason (see https://github.com/hyperledger/cacti/tree/main/packages/cactus-plugin-ledger-connector-iroha2/src/test/typescript). In the end I did not do that (to adhere to general principle that tests should not depend on each other too much and because iroha starts really quickly), but maybe this could be considered for fabric connector where the ledger takes a looong time to start up.
...ger-connector-fabric/src/test/typescript/integration/fabric-v2-2-x/deploy-lock-asset.test.ts
Show resolved
Hide resolved
@outSH Agreed, 100%. Now that I know that there's some support for the idea I'll consolidate a few more tests into one! Stay tuned for an upcoming PR :-) |
043a8b6
to
c664438
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petermetz LGTM, thanks! Please remember to update the commit/PR title to include the new test name, apporving to speed this up :)
c664438
to
43209df
Compare
@outSH Done and done, thank you for catching that and also much appreciated on the pre-approval :-) |
1. This is making the test case harder to read but shaves off easily 10 to 15 minutes from one of our slowest CI jobs which can take up to an hour to run when the GitHub runners are feeling lazy. 2. That above is my only justification for it. The test cases I'm consolidating are relatively stable at this point (took us years to get here but now they are passing with a high ratio and the false negatives have pretty much disappeared). 3. We are downloading and launching the fabirc AIO ledger 10+ times which is very resource intensive and this could help make a dent in it. Running this test right now looks like this on my machine: PASS packages/cactus-plugin-ledger-connector-fabric/src/test/typescript/ integration/fabric-v2-2-x/connector-fabric-baseline.test.ts (277.062 s, 638 MB heap size) PluginLedgerConnectorFabric ✓ getBlockV1() -Get first block by it's number - decoded. (1216 ms) ✓ getBlockV1() - Get first block by it's number - encoded. (1084 ms) ✓ getBlockV1() - Get a block by transactionId it contains (4534 ms) ✓ getBlockV1() - Get a block by transactionId it contains - cacti transactions (4535 ms) ✓ getBlockV1() - Get a block by transactionId it contains - cacti full block (4559 ms) ✓ getBlockV1() - Get block by it's hash. (6727 ms) ✓ getBlockV1() - Reading block with invalid number returns an error. (1 ms) ✓ GetChainInfoV1() - Get test ledger chain info. (2134 ms) ✓ deployContractV1() - deploys Fabric 2.x contract from go source (38351 ms) ✓ deployContractV1() - deploys contract and performs transactions (40840 ms) Test Suites: 1 passed, 1 total Tests: 10 passed, 10 total Snapshots: 0 total Time: 277.117 s Signed-off-by: Peter Somogyvari <[email protected]>
43209df
to
d36c9ae
Compare
10 to 15 minutes from one of our slowest CI jobs which can take up to an
hour to run when the GitHub runners are feeling lazy.
are relatively stable at this point (took us years to get here but now they
are passing with a high ratio and the false negatives have pretty much
disappeared).
is very resource intensive and this could help make a dent in it.
Running this test right now looks like this on my machine:
PASS packages/cactus-plugin-ledger-connector-fabric/src/test/typescript/
integration/fabric-v2-2-x/connector-fabric-baseline.test.ts (277.062 s, 638 MB heap size)
PluginLedgerConnectorFabric
✓ getBlockV1() -Get first block by it's number - decoded. (1216 ms)
✓ getBlockV1() - Get first block by it's number - encoded. (1084 ms)
✓ getBlockV1() - Get a block by transactionId it contains (4534 ms)
✓ getBlockV1() - Get a block by transactionId it contains - cacti transactions (4535 ms)
✓ getBlockV1() - Get a block by transactionId it contains - cacti full block (4559 ms)
✓ getBlockV1() - Get block by it's hash. (6727 ms)
✓ getBlockV1() - Reading block with invalid number returns an error. (1 ms)
✓ GetChainInfoV1() - Get test ledger chain info. (2134 ms)
✓ deployContractV1() - deploys Fabric 2.x contract from go source (38351 ms)
✓ deployContractV1() - deploys contract and performs transactions (40840 ms)
Test Suites: 1 passed, 1 total
Tests: 10 passed, 10 total
Snapshots: 0 total
Time: 277.117 s
Signed-off-by: Peter Somogyvari [email protected]
Pull Request Requirements
upstream/main
branch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.-s
flag when usinggit commit
command. You may refer to this link for more information.Character Limit
A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.